Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ModelPart #84

Merged
merged 37 commits into from
Sep 2, 2020
Merged

Adding ModelPart #84

merged 37 commits into from
Sep 2, 2020

Conversation

philbucher
Copy link
Member

@philbucher philbucher commented Aug 25, 2020

This adds the ModelPart for the CoSimIO, which will serve as data-container esp for exchanging Mesh and (Geometry in the future)
I decided to follow the interface we have in Kratos to not overcomplicate / reinvent the wheel

To be discussed:

  • container-types: private / public ? => public
  • allowing non-const access? => probably not for now but maybe in future => will be necessary in future
  • is using std::size_t ok? => thinking of MPI => typedef to std::ptrdiff_t (for reference: long index discussion in Kratos)
  • implement and add tests after discussing
  • CoSimIO namespace is missing :)
  • add IndexType and CoordinatesType to define.h

@philbucher philbucher added the enhancement New feature or request label Aug 25, 2020
@philbucher philbucher mentioned this pull request Aug 27, 2020
@philbucher
Copy link
Member Author

philbucher commented Aug 27, 2020

more discussion points:

  • Element stored Nodes as vector<Node*> vs vector<reference_wrapper<Node>>. Using them is kinda nasty both times:
// reference_wrapper<Node>
for (auto node_it=element.NodesBegin(); node_it!=element.NodesEnd(); ++node_it) {
    node_it->get().Id();
}

// Node*
for (auto node_it=element.NodesBegin(); node_it!=element.NodesEnd(); ++node_it) {
    (*node_it)->Id();
}

=> maybe we can have our own iterator that dereferences things right away?

  • Do we need the Connectivities / NodeIds function for Element? => might only become clearer when using it => no for now, we add it if we need it :)
  • Having a Nodes function like ModelPart has it will require to expose the container right? (would still be covenient though :)) => Won't we anyway needs sth like this for C and Python? => like in the link above we could provide an aux vector containing reference => although this is a performance overhead :/ => iterators is enough for now! (pybind works nicely with iterators)

@philbucher
Copy link
Member Author

philbucher commented Aug 31, 2020

@pooyan-dadvand I think this is ready. I would merge as is such that I can make the C and the Python interface. Once those are done then we are sure the basic capabilities work across languages and we can refine it / start using it

for future PRs:

  • switch from shared_ptr to intrusive_ptr (as discussed)
  • use an unordered_map as additional data-structure to speed up the Id-based search

@pooyan-dadvand
Copy link
Member

@philbucher I think you are missing the element type in the element and also in the CreateNewElement interface

Copy link
Member Author

@philbucher philbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philbucher I think you are missing the element type in the element and also in the CreateNewElement interface

no it is there


Element(
const IdType I_Id,
const ElementType I_Type,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is here


Element& CreateNewElement(
const IdType I_Id,
const Element::ElementType I_Type,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

@pooyan-dadvand
Copy link
Member

Let me go and prepare myself another coffee! Seems that I need it!

@pooyan-dadvand
Copy link
Member

I just got confused between IdType and I_Type

Copy link
Member

@pooyan-dadvand pooyan-dadvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving after a coffee! 👍

@philbucher philbucher merged commit 0fef9aa into master Sep 2, 2020
@philbucher philbucher deleted the model-part branch September 2, 2020 10:26
@philbucher philbucher mentioned this pull request Sep 9, 2020
35 tasks
@philbucher philbucher mentioned this pull request Jan 13, 2021
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants